-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add delete button for additionalProperties key-value pair #1123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort. I'd like to merge this to clear out the old PR and issue.
I'm a bit concerned about the change of behavior for setting a field value to an empty string. I understand the rationale here but we've had some problems with this empty-form-input => empty string / undefined conversion before. See #289, #539. Introducing different behavior here for additional properties seems like an accident waiting to happen. Is there another approach that could get us what we want?
.gitignore
Outdated
@@ -4,3 +4,4 @@ build | |||
dist | |||
lib | |||
yarn.lock | |||
.vscode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this belongs here. It would be better to put this in your .git/info/exclude
.
src/components/fields/SchemaField.js
Outdated
paddingRight: 10, | ||
fontWeight: "bold", | ||
height: "34px", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project doesn't use inline styles. You can rely on Bootstrap styles.
src/components/fields/ObjectField.js
Outdated
@@ -79,9 +80,21 @@ class ObjectField extends Component { | |||
); | |||
} | |||
|
|||
onPropertyChange = name => { | |||
onPropertyChange = (name, additionalProperties = false) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about naming this parameter isAdditionalProperty
or addedByAdditionalProperties
?
src/components/fields/ObjectField.js
Outdated
@@ -93,6 +106,18 @@ class ObjectField extends Component { | |||
}; | |||
}; | |||
|
|||
onDropIndexClick = index => { | |||
return event => { | |||
if (event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we need to check whether event
exists or not..
src/components/fields/ObjectField.js
Outdated
@@ -200,6 +224,9 @@ class ObjectField extends Component { | |||
TitleField, | |||
DescriptionField, | |||
properties: orderedProperties.map(name => { | |||
const additionalPropertyFlag = schema.properties[name].hasOwnProperty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to rename this variable too.
src/components/fields/ObjectField.js
Outdated
@@ -93,6 +106,18 @@ class ObjectField extends Component { | |||
}; | |||
}; | |||
|
|||
onDropIndexClick = index => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name index
in the names of the prop, the function, and the argument are misleading. This should be a string property name, right?
@glasserc Existence of If emptying a string would not make pair disappear it would be great, but I couldn't find a better place for it - if you know a better place/method how to make it work like it should then please let me know 👍 . After this PR gets merged when can we expect next release? |
Thanks for your contribution. I agree that this is maybe the least bad of the possible solutions to clearing out an input field. I'll cut a release later today. |
* add del button functionality * delete unnecesary console.logs * fix tests * remove unnecesary console.log * Fix disappearing key-value pair and repair delete button * Add .vscode to gitignore * fix tests * Make test work for select/radio button * Add test cases * Remove one file from PR and one helper function * Add one classname for value input * remove accidentally added .vscode * linted codebase * remove inline styles, make use of bootstrap 3 styles and fix related tests * revert changes to .gitignore
{children} | ||
{errors} | ||
{help} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pieplu I agree that lines 134 and 135 of this file can surely be merged into one (mistake on my side), which would probably leave just one empty div which is in line 150, but I at the first glance I don't know how to achieve same appearence using Bootstrap 3, and have a look at this: #1123 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can try PR a fix with bootstrap style
Hi! @aerfio , as I say on this comment: https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files#r249176646 , this PR include useless div, and break some styles. |
* Infer type from enum if a type is not provided to SelectWidget (#1100) Connects to #1098 Change-type: minor Signed-off-by: Lucian <lucian.buzzo@gmail.com> * No more useless div on schema field not additional * Fix tests (remove useles html) - Revert some test added by https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files - use new css class for form-additional insted layout css class for test selection * Don't use WrapIfAdditonal Component to hav a clerer diff, but adding code duplicate * Use a WrapIfAdditional Component to have a cleanner code / diff * Fix tests * Ignore vscode Ide folder * Remove useless parentProps * Fix old test selector for form additional
…team#1158) * Infer type from enum if a type is not provided to SelectWidget (rjsf-team#1100) Connects to rjsf-team#1098 Change-type: minor Signed-off-by: Lucian <lucian.buzzo@gmail.com> * No more useless div on schema field not additional * Fix tests (remove useles html) - Revert some test added by https://github.com/mozilla-services/react-jsonschema-form/pull/1123/files - use new css class for form-additional insted layout css class for test selection * Don't use WrapIfAdditonal Component to hav a clerer diff, but adding code duplicate * Use a WrapIfAdditional Component to have a cleanner code / diff * Fix tests * Ignore vscode Ide folder * Remove useless parentProps * Fix old test selector for form additional
Reasons for making this change
This PR adds delete button for key-value pair in additionalProperties, also rearanges those two inputs + button to be in line, not one below another. It also removes the ability to remove pair by removing content of value input - we have button for that.
I have also fixed tests related to time widget (change from 2018 to 2019 made two tests crash).
Work largely based on this PR: #1085, but since nothing has changed there for some time I decided to make this PR.
This PR would resolve #1099.
Checklist
npm run cs-format
on my branch to conform my code to prettier coding style